Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Env var instrumentation improvements #2088

Merged
merged 37 commits into from
Feb 1, 2023
Merged

Env var instrumentation improvements #2088

merged 37 commits into from
Feb 1, 2023

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Jan 27, 2023

Why

Fixes #1633

What

  • Change the way to manage enabled instrumentations. The following environmental
    variables:

    • OTEL_DOTNET_AUTO_TRACES_ENABLED_INSTRUMENTATIONS,
    • OTEL_DOTNET_AUTO_TRACES_DISABLED_INSTRUMENTATIONS,
    • OTEL_DOTNET_AUTO_METRICS_ENABLED_INSTRUMENTATIONS,
    • OTEL_DOTNET_AUTO_METRICS_DISABLED_INSTRUMENTATIONS,
    • OTEL_DOTNET_AUTO_LOGS_ENABLED_INSTRUMENTATIONS,
    • OTEL_DOTNET_AUTO_LOGS_DISABLED_INSTRUMENTATIONS

    are replaced by:

    • OTEL_DOTNET_AUTO_INSTRUMENTATION_ENABLED,
    • OTEL_DOTNET_AUTO_TRACES_INSTRUMENTATION_ENABLED,
    • OTEL_DOTNET_AUTO_TRACES_{0}_INSTRUMENTATION_ENABLED,
    • OTEL_DOTNET_AUTO_METRICS_INSTRUMENTATION_ENABLED,
    • OTEL_DOTNET_AUTO_METRICS_{0}_INSTRUMENTATION_ENABLED,
    • OTEL_DOTNET_AUTO_LOGS_INSTRUMENTATION_ENABLED,
    • OTEL_DOTNET_AUTO_LOGS_{0}_INSTRUMENTATION_ENABLED.

Tests

CI + updated tests

Checklist

  • CHANGELOG.md is updated.
  • Documentation is updated.
  • New features are covered by tests.

@github-actions github-actions bot requested a review from theletterf January 27, 2023 07:34
@pellared pellared changed the title Env var instrumentation imrpovements Env var instrumentation improvements Jan 27, 2023
@Kielek Kielek marked this pull request as ready for review January 27, 2023 08:51
@Kielek Kielek requested a review from a team January 27, 2023 08:51
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
@pjanotti
Copy link
Contributor

pjanotti commented Jan 27, 2023

🤔 @Kielek IIRC the conversation on the SIG with @carlosalberto was to follow Java in this regard, i.e.: use _ENABLED instead. These will be very similar environment variables and it will be nice to have similar languages following the same pattern.

@carlosalberto what are your thoughts in this regard?

Issue regarding bool env vars in this project: #2090

@pellared
Copy link
Member

@Kielek why going with the _DISABLED instead of _ENABLED with reversed defaults? I ask because Java uses _ENABLED instead _DISABLED. See https://opentelemetry.io/docs/instrumentation/java/automatic/agent-config/#enable-only-specific-instrumentation

See: #2090

@pjanotti
Copy link
Contributor

pjanotti commented Jan 27, 2023

@pellared I'm aware of that but I'm under the impression that at the SIG meeting, we talked with Carlos about following the Java precedent in this case.

@pellared
Copy link
Member

@pellared I'm aware of that but I'm under the impression that at the SIG meeting, we talked with Carlos about following the Java precedent in this case.

Java is still a little different. We have not discussed _DISABLED vs _ENABLED with @carlosalberto. I lean towards spec recommendation, but I will not fight for it 😄

@pjanotti
Copy link
Contributor

@pellared neither I will fight for it :), but, I would prefer to commit the version that has the smaller chance of having to be reverted later.

@theletterf
Copy link
Member

So, what are we going forward with in the end? 😬

@rajkumar-rangaraj
Copy link
Contributor

Even, I feel it is easier for customers to use _ENABLED in the environment variables. We could hold the merge of the PR till SIG meeting for a detailed discussion.

@pellared
Copy link
Member

pellared commented Jan 31, 2023

Reasons why I lean towards _DISABLED

  1. Follows OTel spec recommendation
  2. Same approach as with OTEL_SDK_DISABLED
  3. I have not found any env var in the OTel spec which defaults to true
  4. I think that the convention with leaning towards using false as the default value makes sense as:

I think this approach has smaller chances in getting changed as it is based on OTel spec. That is why I would prefer merging the current version.

@Kielek Kielek requested a review from pellared February 1, 2023 17:56
Copy link
Contributor

@rajkumar-rangaraj rajkumar-rangaraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
left very minor comments, could be addressed in a next PR. Nice work.

@Kielek Kielek merged commit 740b802 into open-telemetry:main Feb 1, 2023
@Kielek Kielek deleted the env-var-instrumentation-imrpovements branch February 1, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select instrumentation libraries via OTEL_DOTNET_AUTO_*_INSTRUMENTATION_ENABLED
6 participants